-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix cppcheck 1.82 warnings #9732
Conversation
Resolve the following uninitialized variable warnings. In practice these were unreachable due to the goto. Replacing the goto with a return resolves the warning and yields more readable code. [module/icp/algs/modes/ccm.c:892]: (error) Uninitialized variable: ccm_param [module/icp/algs/modes/ccm.c:893]: (error) Uninitialized variable: ccm_param [module/icp/algs/modes/gcm.c:564]: (error) Uninitialized variable: gcm_param [module/icp/algs/modes/gcm.c:565]: (error) Uninitialized variable: gcm_param [module/icp/algs/modes/gcm.c:599]: (error) Uninitialized variable: gmac_param [module/icp/algs/modes/gcm.c:600]: (error) Uninitialized variable: gmac_param Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
As of cppcheck 1.82 warnings are issued when using the list_for_each_* functions with an uninitialized variable. Functionally, this is fine but to resolve the warning initialize these variables. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
As of cppcheck 1.82 surpress the warning regarding shifting too many bits for __divdi3() implemention. The algoritm used here is correct. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Resolve the reported memory leak by using a dedicated local vptr variable to store the pointer reported by calloc(). Only assign the passed **vtoc function argument on success, in all other cases vptr is freed. [lib/libefi/rdwr_efi.c:403]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:422]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:440]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:454]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:470]: (error) Memory leak: vtoc Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
The dnp argument can only be set to NULL when the DNODE_DRY_RUN flag is set. In which case, an early return path will be executed and a NULL pointer dereference at the given location is impossible. Add an additional ASSERT to silence the cppcheck warning and document that dbp must never be NULL at the point in the function. [module/zfs/dnode.c:1566]: (warning) Possible null pointer deref: dnp Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
I can confirm this solves the referenced warnings. However, it should be noted that there are still warnings left on Debian 10 with CPPCHECK 1.86:
|
As indicated by the VERIFY the local who_perm variable can never be NULL in parse_fs_perm(). Due to the existance of the is_set conditional, which is always true, cppcheck 1.88 was reporting a possible NULL reference. Resolve the issue by removing the extraneous is_set variable. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Suppress autoVariables warnings in the lua interpretter. The usage here while unconventional in intentional and the same as upstream. [module/lua/ldebug.c:327]: (error) Address of local auto-variable assigned to a function parameter. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Move the 'nvh = (void *)buf' assignment after the 'buf == NULL' check to resolve the warning. Interestingly, cppcheck 1.88 correctly determines that the existing code is safe, while cppcheck 1.86 reports the warning. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
@Ornias1993 thanks for testing this with a newer version of cppcheck. Originally I targeted cppcheck 1.82 since that's what ships with Ubuntu 18.04 and will be used by the CI. But as long we're here I agree it would be nice to resolve everything reported by the latest version. I've gone ahead and added 3 more commits to the PR to address everything reported by cppcheck 1.88. |
Codecov Report
@@ Coverage Diff @@
## master #9732 +/- ##
========================================
+ Coverage 79% 79% +<1%
========================================
Files 420 420
Lines 123654 123657 +3
========================================
+ Hits 98136 98163 +27
+ Misses 25518 25494 -24
Continue to review full report at Codecov.
|
@@ -1559,6 +1559,7 @@ dnode_hold_impl(objset_t *os, uint64_t object, int flag, int slots, | |||
dnode_slots_rele(dnc, idx, slots); | |||
|
|||
DNODE_VERIFY(dn); | |||
ASSERT3P(dnp, !=, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dnp
is also used on line 1329, so should there be an IMPLY(!(flag & DNODE_DRY_RUN), (tag != NULL) && (dnp != NULL));
at the top of the function instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That wouldn't hurt, but I don't think it's really needed in this case. If you feel strongly about it I can it, but I don't think this has resulted in any real confusion (aside from the cppcheck warning).
@behlendorf My pleasure! :) |
True. Probably in 20.04. But for the moment I'll settling for updating the CI to 18.04! @mattmacy would you mind looking over these small changes and verifying that with them applied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just a minor typo in one of your commit messages: "The algoritm used here is correct"
As of cppcheck 1.82 warnings are issued when using the list_for_each_* functions with an uninitialized variable. Functionally, this is fine but to resolve the warning initialize these variables. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
As of cppcheck 1.82 surpress the warning regarding shifting too many bits for __divdi3() implemention. The algorithm used here is correct. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
Resolve the reported memory leak by using a dedicated local vptr variable to store the pointer reported by calloc(). Only assign the passed **vtoc function argument on success, in all other cases vptr is freed. [lib/libefi/rdwr_efi.c:403]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:422]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:440]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:454]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:470]: (error) Memory leak: vtoc Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
The dnp argument can only be set to NULL when the DNODE_DRY_RUN flag is set. In which case, an early return path will be executed and a NULL pointer dereference at the given location is impossible. Add an additional ASSERT to silence the cppcheck warning and document that dbp must never be NULL at the point in the function. [module/zfs/dnode.c:1566]: (warning) Possible null pointer deref: dnp Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
As indicated by the VERIFY the local who_perm variable can never be NULL in parse_fs_perm(). Due to the existence of the is_set conditional, which is always true, cppcheck 1.88 was reporting a possible NULL reference. Resolve the issue by removing the extraneous is_set variable. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
Suppress autoVariables warnings in the lua interpreter. The usage here while unconventional in intentional and the same as upstream. [module/lua/ldebug.c:327]: (error) Address of local auto-variable assigned to a function parameter. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
Move the 'nvh = (void *)buf' assignment after the 'buf == NULL' check to resolve the warning. Interestingly, cppcheck 1.88 correctly determines that the existing code is safe, while cppcheck 1.86 reports the warning. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
Resolve the following uninitialized variable warnings. In practice these were unreachable due to the goto. Replacing the goto with a return resolves the warning and yields more readable code. [module/icp/algs/modes/ccm.c:892]: (error) Uninitialized variable: ccm_param [module/icp/algs/modes/ccm.c:893]: (error) Uninitialized variable: ccm_param [module/icp/algs/modes/gcm.c:564]: (error) Uninitialized variable: gcm_param [module/icp/algs/modes/gcm.c:565]: (error) Uninitialized variable: gcm_param [module/icp/algs/modes/gcm.c:599]: (error) Uninitialized variable: gmac_param [module/icp/algs/modes/gcm.c:600]: (error) Uninitialized variable: gmac_param Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
As of cppcheck 1.82 warnings are issued when using the list_for_each_* functions with an uninitialized variable. Functionally, this is fine but to resolve the warning initialize these variables. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
As of cppcheck 1.82 surpress the warning regarding shifting too many bits for __divdi3() implemention. The algorithm used here is correct. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
Resolve the reported memory leak by using a dedicated local vptr variable to store the pointer reported by calloc(). Only assign the passed **vtoc function argument on success, in all other cases vptr is freed. [lib/libefi/rdwr_efi.c:403]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:422]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:440]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:454]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:470]: (error) Memory leak: vtoc Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
The dnp argument can only be set to NULL when the DNODE_DRY_RUN flag is set. In which case, an early return path will be executed and a NULL pointer dereference at the given location is impossible. Add an additional ASSERT to silence the cppcheck warning and document that dbp must never be NULL at the point in the function. [module/zfs/dnode.c:1566]: (warning) Possible null pointer deref: dnp Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
As indicated by the VERIFY the local who_perm variable can never be NULL in parse_fs_perm(). Due to the existence of the is_set conditional, which is always true, cppcheck 1.88 was reporting a possible NULL reference. Resolve the issue by removing the extraneous is_set variable. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
Suppress autoVariables warnings in the lua interpreter. The usage here while unconventional in intentional and the same as upstream. [module/lua/ldebug.c:327]: (error) Address of local auto-variable assigned to a function parameter. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
Move the 'nvh = (void *)buf' assignment after the 'buf == NULL' check to resolve the warning. Interestingly, cppcheck 1.88 correctly determines that the existing code is safe, while cppcheck 1.86 reports the warning. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
Resolve the following uninitialized variable warnings. In practice these were unreachable due to the goto. Replacing the goto with a return resolves the warning and yields more readable code. [module/icp/algs/modes/ccm.c:892]: (error) Uninitialized variable: ccm_param [module/icp/algs/modes/ccm.c:893]: (error) Uninitialized variable: ccm_param [module/icp/algs/modes/gcm.c:564]: (error) Uninitialized variable: gcm_param [module/icp/algs/modes/gcm.c:565]: (error) Uninitialized variable: gcm_param [module/icp/algs/modes/gcm.c:599]: (error) Uninitialized variable: gmac_param [module/icp/algs/modes/gcm.c:600]: (error) Uninitialized variable: gmac_param Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
As of cppcheck 1.82 warnings are issued when using the list_for_each_* functions with an uninitialized variable. Functionally, this is fine but to resolve the warning initialize these variables. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
As of cppcheck 1.82 surpress the warning regarding shifting too many bits for __divdi3() implemention. The algorithm used here is correct. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
Resolve the reported memory leak by using a dedicated local vptr variable to store the pointer reported by calloc(). Only assign the passed **vtoc function argument on success, in all other cases vptr is freed. [lib/libefi/rdwr_efi.c:403]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:422]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:440]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:454]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:470]: (error) Memory leak: vtoc Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
The dnp argument can only be set to NULL when the DNODE_DRY_RUN flag is set. In which case, an early return path will be executed and a NULL pointer dereference at the given location is impossible. Add an additional ASSERT to silence the cppcheck warning and document that dbp must never be NULL at the point in the function. [module/zfs/dnode.c:1566]: (warning) Possible null pointer deref: dnp Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
As indicated by the VERIFY the local who_perm variable can never be NULL in parse_fs_perm(). Due to the existence of the is_set conditional, which is always true, cppcheck 1.88 was reporting a possible NULL reference. Resolve the issue by removing the extraneous is_set variable. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
Suppress autoVariables warnings in the lua interpreter. The usage here while unconventional in intentional and the same as upstream. [module/lua/ldebug.c:327]: (error) Address of local auto-variable assigned to a function parameter. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
Move the 'nvh = (void *)buf' assignment after the 'buf == NULL' check to resolve the warning. Interestingly, cppcheck 1.88 correctly determines that the existing code is safe, while cppcheck 1.86 reports the warning. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
Resolve the following uninitialized variable warnings. In practice these were unreachable due to the goto. Replacing the goto with a return resolves the warning and yields more readable code. [module/icp/algs/modes/ccm.c:892]: (error) Uninitialized variable: ccm_param [module/icp/algs/modes/ccm.c:893]: (error) Uninitialized variable: ccm_param [module/icp/algs/modes/gcm.c:564]: (error) Uninitialized variable: gcm_param [module/icp/algs/modes/gcm.c:565]: (error) Uninitialized variable: gcm_param [module/icp/algs/modes/gcm.c:599]: (error) Uninitialized variable: gmac_param [module/icp/algs/modes/gcm.c:600]: (error) Uninitialized variable: gmac_param Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
As of cppcheck 1.82 warnings are issued when using the list_for_each_* functions with an uninitialized variable. Functionally, this is fine but to resolve the warning initialize these variables. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9732
Resolve the following uninitialized variable warnings. In practice these were unreachable due to the goto. Replacing the goto with a return resolves the warning and yields more readable code. [module/icp/algs/modes/ccm.c:892]: (error) Uninitialized variable: ccm_param [module/icp/algs/modes/ccm.c:893]: (error) Uninitialized variable: ccm_param [module/icp/algs/modes/gcm.c:564]: (error) Uninitialized variable: gcm_param [module/icp/algs/modes/gcm.c:565]: (error) Uninitialized variable: gcm_param [module/icp/algs/modes/gcm.c:599]: (error) Uninitialized variable: gmac_param [module/icp/algs/modes/gcm.c:600]: (error) Uninitialized variable: gmac_param Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
As of cppcheck 1.82 warnings are issued when using the list_for_each_* functions with an uninitialized variable. Functionally, this is fine but to resolve the warning initialize these variables. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
As of cppcheck 1.82 surpress the warning regarding shifting too many bits for __divdi3() implemention. The algorithm used here is correct. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
Resolve the reported memory leak by using a dedicated local vptr variable to store the pointer reported by calloc(). Only assign the passed **vtoc function argument on success, in all other cases vptr is freed. [lib/libefi/rdwr_efi.c:403]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:422]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:440]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:454]: (error) Memory leak: vtoc [lib/libefi/rdwr_efi.c:470]: (error) Memory leak: vtoc Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
The dnp argument can only be set to NULL when the DNODE_DRY_RUN flag is set. In which case, an early return path will be executed and a NULL pointer dereference at the given location is impossible. Add an additional ASSERT to silence the cppcheck warning and document that dbp must never be NULL at the point in the function. [module/zfs/dnode.c:1566]: (warning) Possible null pointer deref: dnp Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
As indicated by the VERIFY the local who_perm variable can never be NULL in parse_fs_perm(). Due to the existence of the is_set conditional, which is always true, cppcheck 1.88 was reporting a possible NULL reference. Resolve the issue by removing the extraneous is_set variable. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
Suppress autoVariables warnings in the lua interpreter. The usage here while unconventional in intentional and the same as upstream. [module/lua/ldebug.c:327]: (error) Address of local auto-variable assigned to a function parameter. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
Move the 'nvh = (void *)buf' assignment after the 'buf == NULL' check to resolve the warning. Interestingly, cppcheck 1.88 correctly determines that the existing code is safe, while cppcheck 1.86 reports the warning. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9732
Motivation and Context
Resolve new warnings and errors reported by cppcheck 1.82.
This enables us to update the version of cppcheck used by the
CI and is generally needed to keep the
make cppcheck
targetuseful with the latest tool chain.
Description
None of the reported warnings or errors fixed were cause for
concern, but fixing them does improve the overall code quality.
Each patch in this stack addresses a specific class of issue and
is described by its commit message.
84a615f cppcheck: (warning) Possible null pointer dereference: nvh
161704b cppcheck: (error) Address of local auto-variable assigned
3511eb8 cppcheck: (error) Null pointer dereference: who_perm
dd35210 cppcheck: (warning) Possible null pointer dereference: dnp
0f02a61 cppcheck: (error) Memory leak: vtoc
d4fb6f1 cppcheck: (error) Shifting signed 64-bit value by 63 bits
dbd873a cppcheck: (error) Uninitialized variable
1bad165 cppcheck: (error) Uninitialized variable
How Has This Been Tested?
All of the following pass cleanly when testing on Ubuntu 18.04
using the distribution provided version of cppcheck 1.82. Prior
to this change
make cppcheck
would report multiple warnings.Types of changes
Checklist:
Signed-off-by
.